Skip to content

Conversation

@erikolofsson
Copy link
Contributor

Fixes #1638

@BenBE BenBE added bug 🐛 Something isn't working MacOS 🍏 MacOS / Darwin related issues labels May 1, 2025
@erikolofsson erikolofsson requested a review from Explorer09 May 3, 2025 16:06
@Explorer09
Copy link
Contributor

I don't have any more comment with this code now. But perhaps is good to explain what's the difference between this PR and #1643. Does this supersede #1643?

Cc @aestriplex

@erikolofsson erikolofsson requested a review from BenBE May 8, 2025 06:40
@Korrd
Copy link

Korrd commented Jul 16, 2025

Is this fix expected to be merged for next release? I just had to copy old 3.3.0 htop from another mac to mine in order to make it show the right CPU usage. Something like this that makes the software fail to a point it's not showing the right data should be fixed much quicker than several months, as meanwhile it makes it unreliable and unusable for a quite common use-case, which might make users look for alternatives instead, as having the code done right is useless if the product does not work as intended.

@erikolofsson
Copy link
Contributor Author

Rebased on main (comment added to prevent GitHub from merging multiple force pushes)

@Explorer09
Copy link
Contributor

@Korrd Please keep in mind I'm just a volunteer. While I can help evaluting the quality of the patches, I don't have all day to do it and I have no guarantee on which patch would be reviewed sooner or later. (And I'm not the maintainer of this project - this is just my personal position and not the maintainers'.)

@aestriplex
Copy link
Contributor

I don't have any more comment with this code now. But perhaps is good to explain what's the difference between this PR and #1643. Does this supersede #1643?

Cc @aestriplex

@Explorer09 yes, this PR supersede #1643. If it's one with anyone, I can close it

// lowerBound <= currentVersion < upperBound
bool Platform_KernelVersionIsBetween(KernelVersion lowerBound, KernelVersion upperBound);

void Platform_calculateNanosecondsPerMachTick(uint64_t* numer, uint64_t* denom);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are only calling this Platform_calculateNanosecondsPerMachTick() from Platform_init(), I wonder if it's possible to make the former function static scope and not expose this as a public interface in PlatformHelpers.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I would have to move it to Platform.c as it's in another compilation unit otherwise. That file is already rather long. Should I?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no problem with that. Especially as this allows to reduce visibility/exposed interface for a function that is unlikely to get re-used. And regarding large files: There are far more complex ones. And just for comparison: The platform implementation (main part) for Linux is twice the size … What matters more is keeping code that belongs together and is used as an unit near each other; thus with this just being called on platform initialization and all other code related to timebase conversion in Platform.c it's more logical to also have this function over there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, pushed change.

Copy link
Member

@BenBE BenBE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor. Rest seems fine.

@BenBE BenBE added this to the 3.5.0 milestone Jul 16, 2025
@Explorer09
Copy link
Contributor

Explorer09 commented Jul 16, 2025

I slightly reordered the logic of the Platform_calculateNanosecondsPerMachTick function:

static void Platform_calculateNanosecondsPerMachTick(uint64_t* numer, uint64_t* denom) {
   // Check if we can determine the timebase used on this system.
   // If the API is unavailable assume we get our timebase in nanoseconds.
   *numer = 1;
   *denom = 1;

#ifdef __x86_64__
   /* WORKAROUND for `mach_timebase_info` giving incorrect values on M1 under Rosetta 2.
    *    rdar://FB9546856 http://www.openradar.appspot.com/FB9546856
    *
    *    We don't know exactly what feature/attribute of the M1 chip causes this mistake under Rosetta 2.
    *    Until we have more Apple ARM chips to compare against, the best we can do is special-case
    *    the "Apple M1" chip specifically when running under Rosetta 2.
    *
    *    Rosetta 2 only supports x86-64, so skip this workaround when building for other architectures.
    */

   bool isRunningUnderRosetta2 = Platform_isRunningTranslated();

   // Kernel version 20.0.0 is macOS 11.0 (Big Sur)
   bool isBuggedVersion = 0 <= Platform_CompareKernelVersion((KernelVersion) {20, 0, 0});

   if (isRunningUnderRosetta2 && isBuggedVersion) {
      // In this case `mach_timebase_info` provides the wrong value, so we hard-code the correct factor,
      // as determined from `mach_timebase_info` as if the process was running natively.
      *numer = 125;
      *denom = 3;
      return;
   }
#endif

#ifdef HAVE_MACH_TIMEBASE_INFO
   mach_timebase_info_data_t info;
   if (mach_timebase_info(&info) == KERN_SUCCESS) {
      *numer = info.numer;
      *denom = info.denom;
      return;
   }
#endif
}

I feel the code path like this would be easier to follow. LGTM for other parts of the code.

@BenBE
Copy link
Member

BenBE commented Jul 16, 2025

I slightly reordered the logic of the Platform_calculateNanosecondsPerMachTick function:

I feel the code path like this would be easier to follow. LGTM for other parts of the code.

Minor caveat, as with the current way the return;s are written, it looks like there's a path without return value, which looks odd. Instead, a slightly clearer variant to order would be:

static void Platform_calculateNanosecondsPerMachTick(uint64_t* numer, uint64_t* denom) {
   // Check if we can determine the timebase used on this system.

#ifdef __x86_64__
   /* WORKAROUND for `mach_timebase_info` giving incorrect values on M1 under Rosetta 2.
    *    rdar://FB9546856 http://www.openradar.appspot.com/FB9546856
    *
    *    We don't know exactly what feature/attribute of the M1 chip causes this mistake under Rosetta 2.
    *    Until we have more Apple ARM chips to compare against, the best we can do is special-case
    *    the "Apple M1" chip specifically when running under Rosetta 2.
    *
    *    Rosetta 2 only supports x86-64, so skip this workaround when building for other architectures.
    */

   bool isRunningUnderRosetta2 = Platform_isRunningTranslated();

   // Kernel version 20.0.0 is macOS 11.0 (Big Sur)
   bool isBuggedVersion = 0 <= Platform_CompareKernelVersion((KernelVersion) {20, 0, 0});

   if (isRunningUnderRosetta2 && isBuggedVersion) {
      // In this case `mach_timebase_info` provides the wrong value, so we hard-code the correct factor,
      // as determined from `mach_timebase_info` as if the process was running natively.
      *numer = 125;
      *denom = 3;
      return;
   }
#endif

#ifdef HAVE_MACH_TIMEBASE_INFO
   mach_timebase_info_data_t info = { 0 };
   if (mach_timebase_info(&info) == KERN_SUCCESS) {
      *numer = info.numer;
      *denom = info.denom;
      return;
   }
#endif

   // No info on actual timebase found; assume timebase in nanoseconds.
   *numer = 1;
   *denom = 1;
}

That way, the fallback is clearly visible. This also highlights, that both numer and denom need to be assigned for each return path; although with this version it's no longer enforced as well as with the other variant.

@erikolofsson
Copy link
Contributor Author

That way, the fallback is clearly visible. This also highlights, that both numer and denom need to be assigned for each return path; although with this version it's no longer enforced as well as with the other variant.

I applied the suggested fixes

@BenBE BenBE merged commit 8c963bc into htop-dev:main Jul 17, 2025
19 checks passed
@Korrd
Copy link

Korrd commented Jul 18, 2025

Thanks a lot guys! <3

My love to you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug 🐛 Something isn't working MacOS 🍏 MacOS / Darwin related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Htop 3.4.0-dev not showing CPU percentage on apple silicon

5 participants